-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add math/base/special/croundnf
#9632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feat: add math/base/special/croundnf
#9632
Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
Planeshifter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding croundnf! The implementation looks good overall. I found one issue that needs to be addressed before this can be merged - the native addon build will fail due to a missing dependency in manifest.json. See the inline comment for details.
| "dependencies": [ | ||
| "@stdlib/math/base/napi/binary", | ||
| "@stdlib/math/base/special/roundnf", | ||
| "@stdlib/complex/float32/ctor", | ||
| "@stdlib/complex/float32/real", | ||
| "@stdlib/complex/float32/imag" | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "build" task is missing @stdlib/complex/float32/reim as a dependency. The C implementation in src/main.c includes stdlib/complex/float32/reim.h and calls stdlib_complex64_reim(), so this dependency is needed for the native addon to build correctly.
Looking at the equivalent @stdlib/math/base/special/croundn package, its "build" task includes @stdlib/complex/float64/reim in the dependencies.
| "dependencies": [ | |
| "@stdlib/math/base/napi/binary", | |
| "@stdlib/math/base/special/roundnf", | |
| "@stdlib/complex/float32/ctor", | |
| "@stdlib/complex/float32/real", | |
| "@stdlib/complex/float32/imag" | |
| ] | |
| "dependencies": [ | |
| "@stdlib/math/base/napi/binary", | |
| "@stdlib/math/base/special/roundnf", | |
| "@stdlib/complex/float32/ctor", | |
| "@stdlib/complex/float32/reim", | |
| "@stdlib/complex/float32/real", | |
| "@stdlib/complex/float32/imag" | |
| ] |
|
Thanks for catching this, @Planeshifter! I've added |
|
|
||
| // MODULES // | ||
|
|
||
| var addon = require( './../src/addon.node' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to wrap the addon result in a Complex64 instance to match the documented return type and other similar packages like croundn and cceiln. Currently it returns the raw object from the addon (with .re and .im properties) instead of a proper Complex64 instance.
You'll also need to add the Complex64 import at the top of the file.
| var addon = require( './../src/addon.node' ); | |
| var Complex64 = require( '@stdlib/complex/float32/ctor' ); | |
| var addon = require( './../src/addon.node' ); |
| // MAIN // | ||
|
|
||
| /** | ||
| * Rounds each component of a single-precision complex floating-point number to the nearest multiple of \\(10^n\\). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use backticks here instead of LaTeX notation to match the style in croundn/lib/native.js and other lib files:
| * Rounds each component of a single-precision complex floating-point number to the nearest multiple of \\(10^n\\). | |
| * Rounds each component of a single-precision complex floating-point number to the nearest multiple of `10^n`. |
| function croundnf( z, n ) { | ||
| return addon( z, n ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should wrap the addon result in a Complex64 instance:
| function croundnf( z, n ) { | |
| return addon( z, n ); | |
| } | |
| function croundnf( z, n ) { | |
| var v = addon( z, n ); | |
| return new Complex64( v.re, v.im ); | |
| } |
| "libraries": [ | ||
| "-lm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -lm flag isn't needed here since this package doesn't directly use math.h functions - it delegates to roundnf which handles that dependency in its own manifest.
| "libraries": [ | |
| "-lm" | |
| "libraries": [], |
| "libraries": [ | ||
| "-lm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - should be empty:
| "libraries": [ | |
| "-lm" | |
| "libraries": [], |
| "libraries": [ | ||
| "-lm" | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too:
| "libraries": [ | |
| "-lm" | |
| ], | |
| "libraries": [], |
…necessary -lm flags
Resolves None
Description
This pull request:
math/base/special/croundnf.Related Issues
This pull request has the following related issues:
Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
@stdlib-js/reviewers